Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#113 RsWithHeaders must reuse RsWithHeader #210

Merged
merged 6 commits into from
May 5, 2015
Merged

#113 RsWithHeaders must reuse RsWithHeader #210

merged 6 commits into from
May 5, 2015

Conversation

dmzaytsev
Copy link
Contributor

For #113

Got rid code duplication in RsWithHeader and RsWithHeaders according the task description

@dmzaytsev dmzaytsev mentioned this pull request Apr 25, 2015
Closed
@davvd
Copy link

davvd commented Apr 25, 2015

@dmzaytsev Thanks, someone will review your pull request soon

@davvd
Copy link

davvd commented Apr 25, 2015

@pinaf it's yours,please review

pinaf
pinaf reviewed Apr 25, 2015
View reviewed changes
src/test/java/org/takes/rs/RsWithHeadersTest.java
@@ -47,8 +48,8 @@ public void addsHeadersToResponse() throws IOException {
new RsPrint(
new RsWithHeaders(
new RsEmpty(),
"Host: www.example.com ",
"Content-Type: text/xml "
"Host: www.example.com",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmzaytsev let's extract this into a private final String variable.

@pinaf
Copy link

pinaf commented Apr 25, 2015

@dmzaytsev nice :) 3 minor comments above.

@dmzaytsev
Copy link
Contributor Author

@pinaf done :)

@pinaf
Copy link

pinaf commented Apr 25, 2015

@dmzaytsev thanks. this design is very "russian doll" style.

@dmzaytsev
Copy link
Contributor Author

@pinaf how would you solved this problem?

@pinaf
Copy link

pinaf commented Apr 25, 2015

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Apr 25, 2015

@rultor merge

@pinaf Thanks for your request. @yegor256 Please confirm this.

@pinaf
Copy link

pinaf commented Apr 25, 2015

@dmzaytsev I wouldn't reuse the RqWithHeader class. Instead I'd build the headers once in the constructor as a list and just return it through the head method. It would be more efficient and, since the code is so simple, no readability would be lost.

@dmzaytsev
Copy link
Contributor Author

@pinaf build failed .. weird
I had no errors before push

@dmzaytsev
Copy link
Contributor Author

@pinaf use of the RqWithHeader was a condition of the task
anyway thanks for review

@pinaf
Copy link

pinaf commented Apr 25, 2015

@dmzaytsev only with java 8 on travis. seems like a network issue.
yes, you did the task as requested :)

@dmzaytsev
Copy link
Contributor Author

@yegor256 take a look this PR #210 please.
Thank you

@pinaf
Copy link

pinaf commented Apr 28, 2015

@rultor merge

@rultor
Copy link
Collaborator

rultor commented Apr 28, 2015

@rultor merge

@pinaf Thanks for your request. @yegor256 Please confirm this.

@dmzaytsev
Copy link
Contributor Author

@yegor256 this PR needs to be merged
We are waiting 5 days already.
Please pay your attention.

@dmzaytsev
Copy link
Contributor Author

@yegor256 take a look this PR please

@yegor256
Copy link
Owner

yegor256 commented May 4, 2015

@dmzaytsev this is a wrong design. I disagree with @pinaf here and even wrote an article about it. It will be published in a few days, but you can check the draft here: https://raw.githubusercontent.com/yegor256/blog/master/_drafts/ctors.md

Constructors should be light and never (ideally) do anything except assignments.

@pinaf
Copy link

pinaf commented May 5, 2015

@yegor256 what do you mean exactly? I pointed out an opinion but his design was different.

@pinaf
Copy link

pinaf commented May 5, 2015

@yegor256 full composition with one object instance per element in iterable?

@yegor256
Copy link
Owner

yegor256 commented May 5, 2015

@pinaf I'm talking about this comment you made: #210 (comment)

@pinaf
Copy link

pinaf commented May 5, 2015

@yegor256 yeah, I see - I agree with the article. However, you do note he chose the compositional style - kind of - instead of going with my opinion.

@yegor256
Copy link
Owner

yegor256 commented May 5, 2015

@pinaf current implementation using RsWithHeader correctly, making many instances of this class. but this shouldn't happen in ctor, but in actual head() call instead

@pinaf
Copy link

pinaf commented May 5, 2015

@yegor256 ok - so the missing part is the delaying of the iteration, like you point out in the article. thanks!

@dmzaytsev
Copy link
Contributor Author

@yegor256 useful article, thanks!
the logic has been moved to the head() call.
looks good now ?

yegor256
yegor256 reviewed May 5, 2015
View reviewed changes
src/main/java/org/takes/rs/RsWithHeaders.java
for (final CharSequence hdr: headers) {
resp = new RsWithHeader(resp, hdr);
}
final List<String> ret = new ArrayList<String>(1);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get this... why can't you just do return resp.head()?

@yegor256
Copy link
Owner

yegor256 commented May 5, 2015

@dmzaytsev yes, better, but you over-engineered it, I believe. see my comment

@dmzaytsev
Copy link
Contributor Author

@yegor256 you're right
take a look please

@yegor256
Copy link
Owner

yegor256 commented May 5, 2015

@rultor try to merge

@rultor
Copy link
Collaborator

rultor commented May 5, 2015

@rultor try to merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit fe57e6a into yegor256:master May 5, 2015
@rultor
Copy link
Collaborator

rultor commented May 5, 2015

@rultor try to merge

@yegor256 Done! FYI, the full log is here (took me 13min)

@super132 super132 mentioned this pull request May 6, 2015
Closed
@davvd
Copy link

davvd commented May 7, 2015

@elenavolokhova review this ticket please, for compliance with our quality rules

@yegor256
Copy link
Owner

yegor256 commented May 8, 2015

@dmzaytsev @pinaf here is the article published: http://www.yegor256.com/2015/05/07/ctors-must-be-code-free.html

@elenavolokhova
Copy link

@davvd Looks good!

@davvd
Copy link

davvd commented May 8, 2015

@davvd Looks good!

@elenavolokhova thanks for the review and a good remark

@davvd
Copy link

davvd commented May 9, 2015

@pinaf I added 10 mins to @elenavolokhova (for QA review) in transaction 56872197... Thanks so much! Your account was topped up for 25 mins (transaction ID is AP-2NS64077DN239632Y, the task took 264 hours and 25 mins)... you're getting extra minutes here (c=10)... +25 added to your rating, current score is: +7693

@davvd
Copy link

davvd commented May 9, 2015

@rultor deploy now pls

@rultor
Copy link
Collaborator

rultor commented May 9, 2015

@rultor deploy now pls

@davvd OK, I'll try to deploy now. You can check the progress here

@rultor
Copy link
Collaborator

rultor commented May 9, 2015

@rultor deploy now pls

@davvd Done! FYI, the full log is here (took me 11min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
@pinaf
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dmzaytsev @davvd @pinaf @rultor @yegor256 @elenavolokhova